Merge physical plan evaluator and basic planner to main
#612
Conversation
* Additional pipeline abstractions to share more code when testing the CompilerPipeline and future PlannerPipeline
* Add 3 query planner passes. - AST->logical - logical->resolved - lesolved->physical. Not yet integrated with anything--that will come in a future commit.
* Add 3 query planner passes. - AST->logical - logical->resolved -resolved->physical. Not yet integrated with anything--that will come in a future commit.
| */ | ||
| open class SqlException( | ||
| override var message: String, | ||
| val errorCode: ErrorCode, | ||
| val errorContext: PropertyValueMap? = null, | ||
| errorContextArg: PropertyValueMap? = null, |
There was a problem hiding this comment.
Does this need to be nullable (i.e. what does a nullable errorContextArg define)? The errorContext definition on L49 could be redundant since a null errorContextArg will default to an empty property value map.
There was a problem hiding this comment.
It's nullable because it always has been nullable. Changing this to an argument and defaulting the errorContext property to an empty map if errorContextArg is null was a way of simplifying error handling code that reads context properties without changing all of the call sites of this constructor (a few direct, but many indirect) to conform its new signature, which would have resulted in many more noisy diffs in this series of PRs. If you want, I can undo this change in another commit against this PR, but I would consider making this argument non-nullable to be out of scope of this work.
There was a problem hiding this comment.
Ok got it. I don't think it needs to be addressed in this PR. Could you make an issue and link here regarding making errorContextArg non-nullable?
lang/src/org/partiql/lang/eval/builtins/DynamicLookupExprFunction.kt
Outdated
Show resolved
Hide resolved
lang/src/org/partiql/lang/planner/transforms/VariableIdAllocator.kt
Outdated
Show resolved
Hide resolved
lang/test/org/partiql/lang/eval/EvaluatingCompilerOffsetTests.kt
Outdated
Show resolved
Hide resolved
lang/test/org/partiql/lang/planner/transforms/AstToLogicalVisitorTransformTests.kt
Outdated
Show resolved
Hide resolved
Co-authored-by: Alan Cai <[email protected]>
Merges the following PRs to `main`: - #583 - #584 - #587 - #588 - #589 - #609 - #590 - #592 Also, this adds a TODO comment (ea40e4a, requested by @am357 in offline chat) and excludes `PlannerPipeline` from the new aggregate tests which were pulled in when merging from `main` and were not part of the other PRs (34025b3). By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Merges the following PRs to `main`: - #583 - #584 - #587 - #588 - #589 - #609 - #590 - #592 Also, this adds a TODO comment (ea40e4a, requested by @am357 in offline chat) and excludes `PlannerPipeline` from the new aggregate tests which were pulled in when merging from `main` and were not part of the other PRs (34025b3). By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Merges the following PRs to `main`: - #583 - #584 - #587 - #588 - #589 - #609 - #590 - #592 Also, this adds a TODO comment (ea40e4a, requested by @am357 in offline chat) and excludes `PlannerPipeline` from the new aggregate tests which were pulled in when merging from `main` and were not part of the other PRs (34025b3). By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
This will merge the following PRs to
main:UniqueIdResolvertoMetadataResolver#609Also, this adds a TODO comment (ea40e4a, requested by @am357 in offline chat) and excludes
PlannerPipelinefrom the new aggregate tests which were pulled in when merging frommainand were not part of the other PRs (34025b3).By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.